Skip to content

add full screen for embedding page#448

Merged
iceljc merged 1 commit intoSciSharp:mainfrom
iceljc:main
May 4, 2026
Merged

add full screen for embedding page#448
iceljc merged 1 commit intoSciSharp:mainfrom
iceljc:main

Conversation

@iceljc
Copy link
Copy Markdown
Collaborator

@iceljc iceljc commented May 4, 2026

No description provided.

@iceljc iceljc merged commit 7a969eb into SciSharp:main May 4, 2026
@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Add full-screen mode support for embedding pages

✨ Enhancement

Grey Divider

Walkthroughs

Description
• Add full-screen mode support for embedding pages
• Implement dynamic footer and padding visibility toggle
• Move full-screen styling from page-specific to reusable component
• Update type definitions to include fullScreen property
Diagram
flowchart LR
  A["EmbeddingPage Component"] -->|"fullScreen state"| B["Toggle Footer Visibility"]
  A -->|"fullScreen state"| C["Adjust Page Padding"]
  D["EmbeddingInfoModel"] -->|"fullScreen property"| A
  E["Page Components"] -->|"Remove inline styles"| F["Use Component Logic"]
Loading

Grey Divider

File Changes

1. src/lib/common/embedding/EmbeddingPage.svelte ✨ Enhancement +34/-0

Add full-screen mode state and styling logic

• Add fullScreen reactive state variable to manage full-screen mode
• Implement $effect hook to dynamically hide footer and adjust page padding based on fullScreen
 state
• Set fullScreen from embeddingInfo.fullScreen property in menu data
• Include cleanup function to restore original styles when component unmounts

src/lib/common/embedding/EmbeddingPage.svelte


2. src/routes/page/agent/[embed]/[embedType]/+page.svelte ✨ Enhancement +0/-6

Remove inline footer hiding styles

• Remove hardcoded <svelte:head> style block that hid footer
• Rely on EmbeddingPage component to handle full-screen styling dynamically

src/routes/page/agent/[embed]/[embedType]/+page.svelte


3. src/routes/page/knowledge-base/[embed]/[embedType]/+page.svelte ✨ Enhancement +3/-3

Update imports and type annotations

• Update import from $app/stores to $app/state for consistency
• Change JSDoc type annotation from string? to string | undefined
• Update $page reference to use new state API without reactive prefix

src/routes/page/knowledge-base/[embed]/[embedType]/+page.svelte


View more (1)
4. src/lib/helpers/types/pluginTypes.js ✨ Enhancement +1/-0

Add fullScreen property to type definition

• Add fullScreen boolean property to EmbeddingInfoModel typedef
• Enable menu items to specify full-screen mode for embedding pages

src/lib/helpers/types/pluginTypes.js


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented May 4, 2026

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0)

Grey Divider


Action required

1. SSR document access crash 🐞 Bug ☼ Reliability
Description
EmbeddingPage.svelte calls document.querySelector(...) unconditionally inside a $effect, which
can throw ReferenceError: document is not defined when the component is rendered in
SSR/non-browser contexts. This would break rendering of embedding routes and potentially the whole
page response.
Code

src/lib/common/embedding/EmbeddingPage.svelte[R19-22]

+    $effect(() => {
+        const footer = document.querySelector('.footer');
+        const pageContent = document.querySelector('.page-content');
+
Evidence
The new $effect executes immediately and dereferences document before any guard, unlike other
parts of the codebase that explicitly gate effect-time DOM changes with browser checks. This
strongly indicates the codebase expects $effect to be able to run when document may not exist,
so unguarded DOM access is unsafe.

src/lib/common/embedding/EmbeddingPage.svelte[19-22]
src/routes/VerticalLayout/Sidebar.svelte[69-73]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`src/lib/common/embedding/EmbeddingPage.svelte` uses `document.querySelector` inside `$effect` without a browser/SSR guard. In SSR/non-browser contexts, this can throw because `document` is undefined.

### Issue Context
Elsewhere in the codebase, `$effect` blocks that depend on DOM state are gated with `browser` checks, indicating this is the expected safety pattern.

### Fix Focus Areas
- src/lib/common/embedding/EmbeddingPage.svelte[19-47]
- src/routes/VerticalLayout/Sidebar.svelte[69-73]

### Suggested change
- Import `browser` from `$app/environment` and early-return when not in the browser (or wrap the DOM access in `if (!browser) return;`).
- Alternative: move this logic into `onMount` and keep an effect only for toggling once mounted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. fullScreen JSDoc mismatch 🐞 Bug ⚙ Maintainability
Description
EmbeddingInfoModel documents fullScreen as a required boolean, but runtime usage treats it as
optional and defaults it when absent. This creates misleading typing and increases the chance future
code will assume the field always exists.
Code

src/lib/helpers/types/pluginTypes.js[33]

+ * @property {boolean} fullScreen
Evidence
The type definition marks fullScreen as required, but the only consumer reads it via optional
chaining and defaults it (found?.embeddingInfo?.fullScreen || false), implying it may be missing
(e.g., older plugin menu responses). The plugin menu is returned directly from the API (`return
response.data`) with no normalization layer that would guarantee the new field exists.

src/lib/helpers/types/pluginTypes.js[25-34]
src/lib/common/embedding/EmbeddingPage.svelte[60-71]
src/lib/services/plugin-service.js[21-29]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`EmbeddingInfoModel.fullScreen` is documented as required, but actual usage treats it as optional and defaults it.

### Issue Context
`getPluginMenu()` returns raw API data, so older responses may not include the new `fullScreen` field. Keeping the JSDoc required encourages future code to skip defensive checks.

### Fix Focus Areas
- src/lib/helpers/types/pluginTypes.js[25-34]
- src/lib/common/embedding/EmbeddingPage.svelte[60-71]

### Suggested change
- Update the JSDoc to `@property {boolean} [fullScreen]` (or `@property {boolean?} [fullScreen]`) to match observed usage and preserve backward compatibility.
- Optionally keep the existing defaulting logic in EmbeddingPage (`|| false`).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment on lines +19 to +22
$effect(() => {
const footer = document.querySelector('.footer');
const pageContent = document.querySelector('.page-content');

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Ssr document access crash 🐞 Bug ☼ Reliability

EmbeddingPage.svelte calls document.querySelector(...) unconditionally inside a $effect, which
can throw ReferenceError: document is not defined when the component is rendered in
SSR/non-browser contexts. This would break rendering of embedding routes and potentially the whole
page response.
Agent Prompt
### Issue description
`src/lib/common/embedding/EmbeddingPage.svelte` uses `document.querySelector` inside `$effect` without a browser/SSR guard. In SSR/non-browser contexts, this can throw because `document` is undefined.

### Issue Context
Elsewhere in the codebase, `$effect` blocks that depend on DOM state are gated with `browser` checks, indicating this is the expected safety pattern.

### Fix Focus Areas
- src/lib/common/embedding/EmbeddingPage.svelte[19-47]
- src/routes/VerticalLayout/Sidebar.svelte[69-73]

### Suggested change
- Import `browser` from `$app/environment` and early-return when not in the browser (or wrap the DOM access in `if (!browser) return;`).
- Alternative: move this logic into `onMount` and keep an effect only for toggling once mounted.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant